[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: zhijun42
Once this PR has been reviewed and has the lgtm label, please assign spzala for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Hi @zhijun42. Thanks for your PR.

I'm waiting for a github.com member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

/ok-to-test

Haven't dig deep, but let me list things I would look into when reviewing this code:

• How is this different from the previous issue with separation of control vs data response. Requested watcher progress notifications are not synchronised with stream #15220
• How dropping of watch responses impacts client. I expect dropping create will deadlock client.
• Can we add robustness test for this?
• Could we refactor the code to make it easier to understand before we add complexity?
• Is the proposed response architecture the only one that solves the issue? What are it's downsides or alternative?

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 69.85%. Comparing base (b9ba05c) to head (0efd674).

@@            Coverage Diff             @@
##             main   #21065      +/-   ##
==========================================
+ Coverage   69.66%   69.85%   +0.19%     
==========================================
  Files         449      449              
  Lines       38177    38218      +41     
==========================================
+ Hits        26596    26699     +103     
+ Misses      10149    10097      -52     
+ Partials     1432     1422      -10     

/retest

@serathius Thanks for the comprehensive list!

How is this different from the previous issue with separation of control vs data response.

In the previous progress issue, there was race between watch events and control responses, and wrong ordering of revisions could happen. That is fundamentally different from this issue, where client sending watch requests much faster than server's processing rate could cause deadlock. For the ease of reasoning, you can ignore the data responses channel in the server, and just focus on how the control channel is filled up and causes deadlock between client and server data exchange.

How dropping of watch responses impacts client. I expect dropping create will deadlock client.

I assume you meant to say "watch requests" (I'm not dropping responses). You're right, I just realized that dropping create requests is incorrect and will leave the system in a broken state that it can't serve create requests anymore.

I refactor my changes to have two separate send channels and then always keep the watch create requests. I also added an integration test TestV3WatchCancellationStorm to show that the system works fine even when there are a lot of watch cancel and create requests.

Can we add robustness test for this?

I'm not sure what kind of robustness test would bring additional value beyond the integration test I just added.

Could we refactor the code to make it easier to understand before we add complexity?

Good point! I did have to spend many days reading through the codebase to understand the watch implementation. Unfortunately, I couldn't find an effective way to refactor it. That being said, I did do my best to:

• Add many comprehensive comments for both my new changes and the existing components.
• Refactor the client side main run() loop into recvLoop/sendLoop, mirroring the implementation of recvLoop/sendLoop of serverWatchStream at the server side. This makes it clear how the client and server exchange watch requests/responses.

What are downsides or alternative?

Downsides:

• More code paths, harder for new developers to fully grasp (although I did try to make everything readable)
• New behavior: Cancel / progress requests could be dropped during high pressure (without impacting correctness of the system)

Alternatives:

• No changes to the client side. Simply increase the server side ctrlStream buffer size. This makes it harder to trigger the deadlock, but doesn't really solve the issue.
• Enhance the current in-process channel based stream implementation with gRPC-like backpressure flow control. This is not a complete fix though - backpressure can reduce likelihood but doesn’t guarantee the watch client/server state machine can’t form a cyclic wait. And it's a much larger blast radius, and harder to fully implement.

Is this the only solution?

• I cannot guarantee so, but this is the best I can do.

And I see the test case TestCacheLaggingWatcher/pipeline_overflow has been failing for many PRs as well. I think it's not related to my changes here.

Significantly reduced the diffs of this PR.

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed after 21 days if no further activity occurs. Thank you for your contributions.

PR needs rebase.

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: zhijun42
Once this PR has been reviewed and has the lgtm label, please assign spzala for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

/retest-required

Rebase done